types: per comment, avoid copy when t is not bound #32176
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vtjnash
force-pushed
the
jn/25796-fieldtype-comment
branch
from
0c52cc8
to
bccd664
Compare
| @@ -1287,7 +1306,10 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value | |||
| } | |||
| else if (cacheable) { | |||
| // recursively instantiate the types of the fields | |||
| ndt->types = inst_ftypes(ftypes, env, stack); | |||
| if (dt->types == NULL) | |||
| ndt->types = jl_compute_fieldtypes(ndt); | |||
There was a problem hiding this comment.
Curious why this is needed?
There was a problem hiding this comment.
It's reveled from an interaction of the PR with lazy-ftypes (although just the type-stack alone could theoretically run into the same problem with this PR). Previously, on arriving in this function, the dt was always the wrapper object and so the env was already that which will be created by jl_compute_fieldtypes. With the PR, it now could be a partially computed type that just needs to be copied, but that also means it could now be the final parameter application to create a concrete type (where with lazy-ftypes, we are now not been computing the ftypes of that intermediate work).
JeffBezanson
approved these changes
May 29, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As mentioned in #9378, be more careful about not copying objects that are already cached. This should get us the same result, but do strictly less work. And it should address the recursion issue mentioned in #25796, by avoiding making copies of any non-bound objects (which is anything maybe-cacheable) now. It's harder to trigger this now that #31877 is merged, but I think still worthwhile to do the cleanup (and to rely less on that optimization).